Leans on fixed-encoding for header fields of V2 envelope#4752
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1344c1e99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| for update in batch.updates.iter() { | ||
| vqueue_meta.apply_update(update); | ||
| if let Err(err) = update.replace_from_slice(op) { |
There was a problem hiding this comment.
This merge path now assumes every RocksDB operand is a single vqueue_table::metadata::Update, but previous versions wrote VQueueMetaUpdates batches. During restart/rolling upgrade, old operands can still exist in WAL/memtables/SST merge chains; decoding them with update.replace_from_slice(op) will fail (or be ignored as non-updates), so metadata merges return None or silently drop updates. That can leave vqueue metadata stale or unreadable until manual repair. Keep a compatibility decode path for the legacy batched operand format until all old operands are drained.
Useful? React with 👍 / 👎.
| #[bilrost(tag(2), encoding(fixed))] | ||
| kind: CommandKind, | ||
| /// Payload codec | ||
| #[bilrost(3)] | ||
| #[bilrost(tag(3), encoding(fixed))] |
There was a problem hiding this comment.
Switching header fields to encoding(fixed) changes their wire type from varint to fixed-width for the same tags, so previously persisted v2 WAL records are no longer decoded with the same schema. Partition replay first decodes as v2::Envelope<Raw> and only falls back to v1 on type mismatch (not decode errors), so old v2 records can fail recovery after upgrade. This needs dual-format decode support or a versioned format/migration boundary.
Useful? React with 👍 / 👎.
…l merges Partial merges didn't add value and wasted precious CPU cycles since the updates cannot be merged This change removes it and increase the limit of successive merges from 100 (way too low for vqueus) to 5000100 (way too low for vqueus) to 5000 which experimentally seems to be enough, but we may expose this as an advanced config option in the future if really needed.
Improves decoding throughput of successive vqueue meta merge updates by ~45% as shows in the benchmark:
```console
vqueue_meta_merge/full_merge_10000_operands
time: [200.41 µs 201.33 µs 202.22 µs]
thrpt: [49.451 Melem/s 49.670 Melem/s 49.897 Melem/s]
change:
time: [-29.998% -29.504% -28.958%] (p = 0.00 < 0.05)
thrpt: [+40.761% +41.852% +42.853%]
Performance has improved.
```
Throughput of "invoke" workload improves 9k/s -> ~11k/s (the relative numbers are what matters here) The assumption is that we will perform a one-shot migration of data in this table that has a value as part of the VQ migration pack.
Additionally, this introduces a fixed encoding for `U128` that encodes the value as plainbytes `[u8;16]` (little-endian). The implementation of U128 has been changed to reflect this structure while keeping the general encoding as changed (a pair of u64 values that are varint encoded).
2 participants
Additionally, this introduces a fixed encoding for
U128that encodes the value as plainbytes[u8;16](little-endian). The implementation of U128 has been changed to reflect this structure while keeping the general encoding as changed (a pair of u64 values that are varint encoded).Stack created with Sapling. Best reviewed with ReviewStack.